Hmm, I looked into this a bit more, and I think the main bug is in ESPAsyncWebServer instead.

This PR only prevents the AsyncClient from going into a state where _tx_unacked_len is negative. But if the value of _tx_unacked_len is invalid, we AFAIK don't know what value would be the correct value to set it to (so the backend+frontend could get into a weird state where each thinks something else for the unacked value).

Here

https://github.com/me-no-dev/ESPAsyncWebServer/blob/f13685ee97675be2ac9502d177d3024ebc49c1e0/src/AsyncEventSource.cpp#L145-L148

send() is only execute if canSend() is true, but that's wrong. send() calls tcp_output which will try to send out data if it can immediately. If not, the sending will be done later automatically. So calling ->send() is valid at any point and doesn't put us into an undefined state.

In the more mature WebSocket code here

https://github.com/me-no-dev/ESPAsyncWebServer/blob/f13685ee97675be2ac9502d177d3024ebc49c1e0/src/AsyncWebSocket.cpp#L54-L120

we see how the send() behavior should be. Here, send() gets called unconditionally after the add().

I will submit a PR in our ESPAsyncWebserver fork to fix the EventSource.

The change here are still good though - it prevents the socket from going into a bad state when send() is not called correctly. Because we don't know what state it should really be in though, I'd recommend logging this event with warning level.

That's debatable. I don't think it deserves much thinking. But how would you document the "canSend()" function?

The name canSend definitely is poorly chosen. It checks if a send() call would do anything (*), but send() can be called any time (See also tcp_output).

What send() does (or should do) is tell LwIP that there's new data in the buffer. Actually, you don't even need send(), as LwIP will send stuff periodically anyway when it has been added before. So the name send() is also a bit misleading.

The whole _tx_unsent_len concept is anyway quite unnecessary, and probably just meant as a sanity check so that users don't forget to call send(). We could also just increment _tx_unacked_len in :add() and remove _tx_unsent_len.

(my main issue is that _tx_unsent_len is useless and can only cause us trouble; if we just add to _tx_unacked_len directly the code will work fine, and don't have to deal with cases where _tx_unsent_len is in a bad state)